feat: Add F4 Diagnostics Linter Panel#117
Conversation
📝 WalkthroughWalkthroughThe PR adds a unified diagnostics schema, Ruff-backed diagnostics service, editor integration, and a new F4 diagnostics panel. It also updates diagnostics documentation, navigation, highlight handling, and tests for parsing, refresh scheduling, rendering, details popups, and jump behavior. ChangesDiagnostics subsystem
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ecli/core/Ecli.py`:
- Around line 5442-5516: The goto_diagnostic method in Ecli.py is slightly over
the cognitive complexity limit because it repeats the same
validate/status/log/return flow in multiple branches. Extract the repeated
failure handling into a small helper such as _report_nav_failure (or equivalent)
and use it for the missing-file, open-file, line-out-of-range, and
column-out-of-range paths while keeping goto_diagnostic focused on navigation.
Keep the existing redraw contract and behavior intact, and use the helper from
goto_diagnostic to reduce branching without changing the success path.
In `@src/ecli/diagnostics/display.py`:
- Around line 37-48: The truncate_middle helper can exceed the requested width
by one character when max_width is 4, which can break callers like _fit_location
that rely on exact column budgeting. Update truncate_middle in display.py to add
a <= 5 early-return guard before the split-and-ellipsis logic, so very small
widths never produce output longer than max_width. Keep the fix localized to
truncate_middle and preserve the existing behavior for wider strings.
In `@src/ecli/diagnostics/models.py`:
- Around line 21-45: Remove order=True from the Diagnostic dataclass in
Diagnostic, since sort_diagnostics() already uses Diagnostic.sort_key() for
canonical ordering. Keep frozen=True, and ensure any comparisons rely on the
explicit sort key instead of the generated dataclass ordering so code fields
with None do not trigger TypeError.
In `@src/ecli/diagnostics/ruff_provider.py`:
- Around line 266-269: The severity mapping in _severity_for_code currently
treats Ruff diagnostics with a missing code as warnings, which causes syntax
parse failures to be underreported. Update the Ruff diagnostics handling in
ruff_provider.py so syntax diagnostics are identified explicitly, rather than
depending only on rule codes in _severity_for_code, and map those parse/syntax
failures to error while keeping normal rule-based severity behavior intact.
In `@src/ecli/diagnostics/service.py`:
- Around line 169-210: The `_run_providers` method in `DiagnosticsService` mixes
result accumulation with final status/message resolution, pushing its cognitive
complexity over the limit. Extract the branching logic that determines the final
status and message into a small module-level helper such as
`_resolve_status_message(...)`, and keep `_run_providers` focused on iterating
providers, collecting diagnostics/messages, and calling `sort_diagnostics(...)`
before building the final `DiagnosticResult`.
In `@src/ecli/ui/panels.py`:
- Around line 2386-2449: The diagnostic row fitting in _format_diagnostic,
_fit_diagnostic_row, and _fit_location is using len() for terminal geometry
instead of display width, which can mis-size Unicode text in curses panels.
Update all width comparisons and truncation decisions in these helpers to use
wcwidth/wcswidth-aware measurements and matching truncate helpers so rendered
rows stay within the panel width. Make the same adjustment in the related
row/detail fit logic referenced by the comment so all diagnostic display paths
use consistent display-width calculations.
- Around line 2462-2470: The _diagnostics_project_root helper currently prefers
editor.filename and returns the active file’s directory before considering
project_service.root, which causes workspace diagnostics to anchor to the wrong
location. Update _diagnostics_project_root in the panels.py logic to read
service_registry.project_service.root first, and only fall back to deriving a
path from editor.filename when the project root is unavailable.
- Around line 2608-2622: Keep the selected diagnostic highlight tied to the
snapshot passed into _sync_selected_diagnostic_highlight() instead of re-reading
state via _selected_diagnostic(). Use the supplied diagnostics tuple to resolve
the selected Diagnostic, then pass that same object to
set_diagnostic_line_highlight with generation=snapshot.generation so the
highlight cannot drift across async refreshes.
- Around line 2263-2273: The refresh handler in _request_refresh currently
returns the bridge’s raw result, which can let handled keys fall through when
request_diagnostics_refresh returns None or False. Update _request_refresh so it
always reports the refresh key as consumed once the panel has processed it: keep
the existing cleanup and bridge call, but return a handled/true result
regardless of the requester’s return value. Use _request_refresh and
request_diagnostics_refresh as the main touchpoints.
- Around line 2191-2245: The draw() method in panels.py is doing too much work
each frame and formats every diagnostic before slicing, which causes unnecessary
UI-thread overhead for large result sets. Refactor draw() by splitting the
status/scroll/list rendering logic into smaller helpers, and change the
diagnostics rendering path so only the visible diagnostics in the current
viewport are passed through _format_diagnostic() before drawing. Keep the
behavior of draw(), _status_lines_with_viewport(), and _draw_details_popup()
unchanged while reducing the cognitive complexity of draw().
In `@tests/ui/test_diagnostics_panel.py`:
- Around line 410-413: The tests in this block are still using literal temp
filenames like /tmp/f4_bad.py and /tmp/example.py instead of the existing
tmp_path fixture. Update the affected test setup in
test_pass_status_message_uses_success_style_hook and the other matching tests in
this file to derive filenames from tmp_path, keeping the same fake-editor
behavior while removing hardcoded publicly-writable paths. Use the same pattern
already used elsewhere in tests/ui/test_diagnostics_panel.py so the suite stays
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cb277273-cbcb-4bec-b2ba-a67cfe7609f6
📒 Files selected for processing (15)
docs/extensions/diagnostics-model.mdsrc/ecli/core/Ecli.pysrc/ecli/diagnostics/__init__.pysrc/ecli/diagnostics/display.pysrc/ecli/diagnostics/models.pysrc/ecli/diagnostics/ruff_provider.pysrc/ecli/diagnostics/service.pysrc/ecli/integrations/LinterBridge.pysrc/ecli/ui/DrawScreen.pysrc/ecli/ui/KeyBinder.pysrc/ecli/ui/PanelManager.pysrc/ecli/ui/panels.pytests/characterization/test_existing_keybindings.pytests/core/test_diagnostics_service.pytests/ui/test_diagnostics_panel.py
| def truncate_middle(text: str, max_width: int) -> str: | ||
| """Truncate text in the middle using ASCII dots.""" | ||
| if max_width <= 0: | ||
| return "" | ||
| if len(text) <= max_width: | ||
| return text | ||
| if max_width <= 3: | ||
| return text[:max_width] | ||
| keep = max_width - 3 | ||
| left = max(1, keep // 2) | ||
| right = max(1, keep - left) | ||
| return f"{text[:left]}...{text[-right:]}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\btruncate_middle\s*\(' --type=py -C2Repository: SSobol77/ecli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files of interest:\n'
git ls-files 'src/ecli/diagnostics/*' 'src/ecli/*' | sed -n '1,200p'
printf '\nDefinitions/usages of truncate_middle:\n'
rg -n "truncate_middle" src -S
printf '\nSearch for width calculations in diagnostics UI code:\n'
rg -n "max_width|width" src/ecli/diagnostics src/ecli -S | sed -n '1,240p'Repository: SSobol77/ecli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'display.py excerpt:\n'
sed -n '1,120p' src/ecli/diagnostics/display.py | cat -n
printf '\npanels.py excerpt around _fit_location:\n'
sed -n '2400,2460p' src/ecli/ui/panels.py | cat -n
printf '\npanels.py excerpt around caller:\n'
sed -n '2380,2445p' src/ecli/ui/panels.py | cat -nRepository: SSobol77/ecli
Length of output: 9589
truncate_middle can overflow by one column at max_width == 4. src/ecli/ui/panels.py::_fit_location() can pass that width when a long path is being squeezed into the location column. Add a <= 5 guard before the split logic so the output always stays within budget.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/diagnostics/display.py` around lines 37 - 48, The truncate_middle
helper can exceed the requested width by one character when max_width is 4,
which can break callers like _fit_location that rely on exact column budgeting.
Update truncate_middle in display.py to add a <= 5 early-return guard before the
split-and-ellipsis logic, so very small widths never produce output longer than
max_width. Keep the fix localized to truncate_middle and preserve the existing
behavior for wider strings.
| @dataclass(frozen=True, order=True) | ||
| class Diagnostic: | ||
| """One normalized diagnostic item shown by ECLI.""" | ||
|
|
||
| file_path: str | ||
| line: int | ||
| column: int | ||
| severity: DiagnosticSeverity | ||
| code: str | None | ||
| message: str | ||
| source: str | ||
| fix_hint: str | None = None | ||
| suggested_code: str | None = None | ||
|
|
||
| def sort_key(self) -> tuple[int, str, int, int, str, str, str]: | ||
| """Return the deterministic ordering key for diagnostics.""" | ||
| return ( | ||
| _SEVERITY_ORDER[self.severity], | ||
| self.file_path, | ||
| self.line, | ||
| self.column, | ||
| self.source, | ||
| self.code or "", | ||
| self.message, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP '\bsorted\s*\([^)]*\)' --type=py -g '!**/test*' | rg -i diagnostic
rg -nP '(<|>|<=|>=)\s*(self\.)?diagnostic' --type=py -C1Repository: SSobol77/ecli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files | rg '^src/ecli/diagnostics/|^src/.*\.py$' | sed -n '1,200p'
printf '\n== Diagnostic definition ==\n'
nl -ba src/ecli/diagnostics/models.py | sed -n '1,120p'
printf '\n== Diagnostic usages ==\n'
rg -n '\bDiagnostic\b' src -g '*.py'
printf '\n== sort/sorted usages mentioning diagnostics ==\n'
rg -n '\bsorted\s*\(|\.sort\(' src -g '*.py' | rg -i 'diagnostic|sort_key|severity|file_path'
printf '\n== comparisons involving diagnostics ==\n'
rg -n '(<|>|<=|>=|==|!=)\s*(self\.)?(diagnostic|Diagnostic)\b' src -g '*.py' -C 1Repository: SSobol77/ecli
Length of output: 2679
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Diagnostic model ==\n'
sed -n '1,140p' src/ecli/diagnostics/models.py | cat -n
printf '\n== Diagnostic references ==\n'
rg -n '\bDiagnostic\b' src -g '*.py'
printf '\n== sort/sorted uses in diagnostics ==\n'
rg -n '\bsorted\s*\(|\.sort\(' src/ecli -g '*.py'
printf '\n== comparisons involving Diagnostic ==\n'
rg -n '(<|>|<=|>=|==|!=)\s*(self\.)?(diagnostic|Diagnostic)\b' src -g '*.py' -C 2
printf '\n== sort_key / sort_diagnostics ==\n'
rg -n '\bsort_key\b|\bsort_diagnostics\b' src/ecli -g '*.py' -C 2Repository: SSobol77/ecli
Length of output: 14116
🏁 Script executed:
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`(order=True)
class Diagnostic:
file_path: str
line: int
column: int
severity: str
code: str | None
message: str
source: str
a = Diagnostic("a", 1, 1, "error", None, "x", "ruff")
b = Diagnostic("a", 1, 1, "error", "E001", "x", "ruff")
try:
print(a < b)
except Exception as exc:
print(type(exc).__name__, exc)
PYRepository: SSobol77/ecli
Length of output: 221
Drop order=True from Diagnostic. sort_diagnostics() already provides the canonical ordering, and the generated comparison methods can raise TypeError when code is None vs str. No code in the repo relies on Diagnostic’s natural ordering, so frozen=True is enough.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/diagnostics/models.py` around lines 21 - 45, Remove order=True from
the Diagnostic dataclass in Diagnostic, since sort_diagnostics() already uses
Diagnostic.sort_key() for canonical ordering. Keep frozen=True, and ensure any
comparisons rely on the explicit sort key instead of the generated dataclass
ordering so code fields with None do not trigger TypeError.
| def _severity_for_code(code: str | None) -> DiagnosticSeverity: | ||
| if code and (code in _ERROR_CODES or code.startswith("E9")): | ||
| return "error" | ||
| return "warning" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Ruff check JSON output syntax error code "invalid-syntax" vs "E999"
💡 Result:
In Ruff, the error code "E999" was the historical identifier for syntax errors, while "invalid-syntax" is the current terminology used following the removal of E999 as a configurable rule [1][2]. Key distinctions and context include: 1. Deprecation and Removal: E999 was originally used to report syntax errors [3][2]. To ensure that syntax errors are always reported and cannot be silenced, Ruff deprecated and subsequently removed E999 as a formal rule [3][4][2]. 2. JSON Output Changes: With the removal of E999, the JSON output format for syntax errors was updated [5][4]. Fields such as "code", "noqa_row", and "url" are now set to null for syntax errors, reflecting that they are no longer treated as standard configurable lint rules [5][4]. 3. Impact on Integrations: Because E999 has been removed, tools or integrations (such as language servers) that previously relied on checking for the "E999" error code must be updated to handle the new state or the identifier "invalid-syntax" [1][6]. If an integration expects "E999" to appear in the JSON output, it will no longer find it; instead, it must handle the diagnostic appropriately based on the updated structure where the code field is null [5][4]. Essentially, "E999" no longer exists as an active rule in Ruff, and syntax errors are now mandatory, non-suppressible diagnostics [3][4][2].
Citations:
- 1: "E999" -> "invalid-syntax" since ruff 0.8.0 (?) python-lsp/python-lsp-ruff#111
- 2: https://docs.astral.sh/ruff/rules/syntax-error/
- 3: https://astral.sh/blog/ruff-v0.5.0
- 4: Remove
E999as a rule, disallow any disablement methods for syntax error astral-sh/ruff#11901 - 5: astral-sh/ruff@e7b4969
- 6: fix: Replace deprecated E999 error python-lsp/python-lsp-ruff#112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the provider and relevant tests, then inspect only the relevant slices.
git ls-files 'src/ecli/diagnostics/ruff_provider.py' 'tests/core/test_diagnostics_service.py'
printf '\n--- ruff_provider.py outline ---\n'
ast-grep outline src/ecli/diagnostics/ruff_provider.py --view expanded || true
printf '\n--- test_diagnostics_service.py outline ---\n'
ast-grep outline tests/core/test_diagnostics_service.py --view expanded || true
printf '\n--- ruff_provider.py relevant lines ---\n'
sed -n '240,290p' src/ecli/diagnostics/ruff_provider.py
printf '\n--- test_diagnostics_service.py relevant lines ---\n'
sed -n '70,100p' tests/core/test_diagnostics_service.pyRepository: SSobol77/ecli
Length of output: 5163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use read-only probes to determine actual Ruff JSON syntax-error code(s)
# from docs and package behavior context, without executing repository code.
python3 - <<'PY'
import json
import textwrap
samples = [
# historical style
{"code": "E999"},
# current style
{"code": "invalid-syntax"},
# null-style syntax diagnostics seen in some integrations
{"code": None},
]
for s in samples:
print(json.dumps(s))
PYRepository: SSobol77/ecli
Length of output: 210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Ruff provider and test fixture around syntax-error handling.
sed -n '1,220p' src/ecli/diagnostics/ruff_provider.py
printf '\n---\n'
sed -n '1,130p' tests/core/test_diagnostics_service.pyRepository: SSobol77/ecli
Length of output: 11238
🌐 Web query:
Ruff syntax-error JSON diagnostics code invalid-syntax E999 code field null
💡 Result:
In recent versions of Ruff, the error code E999 has been removed and syntax errors are no longer treated as standard linter rules [1][2]. This change, which culminated in its formal removal in version 0.8.0, means that syntax errors are always reported and cannot be suppressed, ignored, or disabled via configuration [3][1]. As a consequence of this change, the JSON diagnostic output for syntax errors reflects the absence of a rule code: the code field in the JSON output is set to null [3][4][5]. This distinguishes syntax errors from standard linting violations, which typically have a string-based rule identifier (e.g., F401) in the code field [3][6]. While some older integrations or documentation may refer to the invalid-syntax identifier or E999, the current behavior is that syntax errors do not carry a rule code, resulting in a null value in the diagnostic JSON [3][5][7]. Integrations that previously relied on checking for code == "E999" must be updated to handle the null code value or to specifically look for syntax error diagnostics as a separate category [8][9][5].
Citations:
- 1: https://docs.astral.sh/ruff/rules/syntax-error/
- 2: https://github.com/astral-sh/ruff/blob/main/crates%2Fruff_linter%2Fsrc%2Frules%2Fpycodestyle%2Frules%2Ferrors.rs
- 3: Remove
E999as a rule, disallow any disablement methods for syntax error astral-sh/ruff#11901 - 4: astral-sh/ruff@e7b4969
- 5: SyntaxError is unable to return code on Linux astral-sh/ruff#12271
- 6: https://github.com/astral-sh/ruff/blob/99d97bd7/crates/ruff_db/src/diagnostic/render/json.rs
- 7: astral-sh/ruff@e9b0c33
- 8: "E999" -> "invalid-syntax" since ruff 0.8.0 (?) python-lsp/python-lsp-ruff#111
- 9: Update deprecated e999 check python-lsp/python-lsp-ruff#93
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show only the relevant regions around severity mapping and the test fixture.
sed -n '250,280p' src/ecli/diagnostics/ruff_provider.py
printf '\n---\n'
sed -n '80,95p' tests/core/test_diagnostics_service.pyRepository: SSobol77/ecli
Length of output: 1521
Treat Ruff syntax errors as error. Ruff JSON syntax diagnostics can come through with code: null, so _severity_for_code currently falls back to warning for blocking parse failures. Handle syntax diagnostics explicitly instead of relying only on rule codes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/diagnostics/ruff_provider.py` around lines 266 - 269, The severity
mapping in _severity_for_code currently treats Ruff diagnostics with a missing
code as warnings, which causes syntax parse failures to be underreported. Update
the Ruff diagnostics handling in ruff_provider.py so syntax diagnostics are
identified explicitly, rather than depending only on rule codes in
_severity_for_code, and map those parse/syntax failures to error while keeping
normal rule-based severity behavior intact.
| def _request_refresh(self, scope: str) -> bool: | ||
| bridge = getattr(self.editor, "linter_bridge", None) | ||
| requester = getattr(bridge, "request_diagnostics_refresh", None) | ||
| if not callable(requester): | ||
| self.editor._set_status_message("Diagnostics service is not available.") | ||
| return True | ||
| self._clear_details_popup() | ||
| self._clear_diagnostic_line_highlight() | ||
| self._clear_legacy_lint_popup() | ||
| self._last_navigation_message = None | ||
| return bool(requester(scope=scope)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Always consume handled refresh keys.
Line 2273 returns the bridge result directly; if the requester returns None or False for a coalesced request, r/R is reported unhandled and can fall through to other input handling even though the panel already processed it.
Suggested fix
- return bool(requester(scope=scope))
+ requester(scope=scope)
+ return True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _request_refresh(self, scope: str) -> bool: | |
| bridge = getattr(self.editor, "linter_bridge", None) | |
| requester = getattr(bridge, "request_diagnostics_refresh", None) | |
| if not callable(requester): | |
| self.editor._set_status_message("Diagnostics service is not available.") | |
| return True | |
| self._clear_details_popup() | |
| self._clear_diagnostic_line_highlight() | |
| self._clear_legacy_lint_popup() | |
| self._last_navigation_message = None | |
| return bool(requester(scope=scope)) | |
| def _request_refresh(self, scope: str) -> bool: | |
| bridge = getattr(self.editor, "linter_bridge", None) | |
| requester = getattr(bridge, "request_diagnostics_refresh", None) | |
| if not callable(requester): | |
| self.editor._set_status_message("Diagnostics service is not available.") | |
| return True | |
| self._clear_details_popup() | |
| self._clear_diagnostic_line_highlight() | |
| self._clear_legacy_lint_popup() | |
| self._last_navigation_message = None | |
| requester(scope=scope) | |
| return True |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/ui/panels.py` around lines 2263 - 2273, The refresh handler in
_request_refresh currently returns the bridge’s raw result, which can let
handled keys fall through when request_diagnostics_refresh returns None or
False. Update _request_refresh so it always reports the refresh key as consumed
once the panel has processed it: keep the existing cleanup and bridge call, but
return a handled/true result regardless of the requester’s return value. Use
_request_refresh and request_diagnostics_refresh as the main touchpoints.
| def _format_diagnostic(self, diagnostic: Diagnostic) -> str: | ||
| severity = self._SEVERITY_LABELS.get(diagnostic.severity, "?") | ||
| source = truncate_end(str(diagnostic.source or "?"), 12) | ||
| prefix = f"{severity} {source} " | ||
| path = self._display_path(diagnostic.file_path) | ||
| location_suffix = f":{diagnostic.line}:{diagnostic.column}" | ||
| message = self._row_message(diagnostic) | ||
| content_width = max(1, self.width - 2) | ||
|
|
||
| full_row = f"{prefix}{path}{location_suffix} {message}" | ||
| if len(full_row) <= content_width: | ||
| return full_row | ||
| return self._fit_diagnostic_row( | ||
| prefix=prefix, | ||
| path=path, | ||
| location_suffix=location_suffix, | ||
| message=message, | ||
| content_width=content_width, | ||
| ) | ||
|
|
||
| def _fit_diagnostic_row( | ||
| self, | ||
| *, | ||
| prefix: str, | ||
| path: str, | ||
| location_suffix: str, | ||
| message: str, | ||
| content_width: int, | ||
| ) -> str: | ||
| if content_width <= len(prefix): | ||
| return truncate_end(prefix, content_width) | ||
|
|
||
| usable = content_width - len(prefix) | ||
| full_location = f"{path}{location_suffix}" | ||
| if usable <= len(location_suffix) + 2: | ||
| return truncate_end(f"{prefix}{full_location}", content_width) | ||
|
|
||
| min_message_width = min(len(message), max(7, content_width // 4)) | ||
| location_width = min( | ||
| len(full_location), | ||
| max(len(location_suffix) + 1, usable - min_message_width - 1), | ||
| ) | ||
| message_width = max(1, usable - location_width - 1) | ||
| if message_width < min_message_width and location_width > len(location_suffix) + 1: | ||
| deficit = min_message_width - message_width | ||
| shrink_by = min(deficit, location_width - len(location_suffix) - 1) | ||
| location_width -= shrink_by | ||
| message_width += shrink_by | ||
|
|
||
| location = self._fit_location(path, location_suffix, location_width) | ||
| message_width = max(1, usable - len(location) - 1) | ||
| row = f"{prefix}{location} {truncate_end(message, message_width)}" | ||
| return truncate_end(row, content_width) | ||
|
|
||
| def _fit_location(self, path: str, suffix: str, max_width: int) -> str: | ||
| location = f"{path}{suffix}" | ||
| if len(location) <= max_width: | ||
| return location | ||
| basename = Path(path).name or path | ||
| basename_location = f"{basename}{suffix}" | ||
| if len(basename_location) <= max_width: | ||
| return basename_location | ||
| path_width = max(1, max_width - len(suffix)) | ||
| return f"{truncate_middle(basename, path_width)}{suffix}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Use display width, not len(), for curses fitting.
These row/detail fit paths use character counts while rendering to fixed terminal cells. Unicode paths or diagnostic messages can exceed the panel width despite passing the len() checks, causing clipped rows, border overwrite, or caught curses.error drops. Use wcswidth/wcwidth-aware truncation for all width comparisons and truncation here. As per coding guidelines, “Rendering risk is likely concentrated around: (1) multiple code paths mutating curses or terminal state, (2) len()-based geometry where display width is required.”
Also applies to: 2572-2578
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/ui/panels.py` around lines 2386 - 2449, The diagnostic row fitting
in _format_diagnostic, _fit_diagnostic_row, and _fit_location is using len() for
terminal geometry instead of display width, which can mis-size Unicode text in
curses panels. Update all width comparisons and truncation decisions in these
helpers to use wcwidth/wcswidth-aware measurements and matching truncate helpers
so rendered rows stay within the panel width. Make the same adjustment in the
related row/detail fit logic referenced by the comment so all diagnostic display
paths use consistent display-width calculations.
Source: Coding guidelines
| def _diagnostics_project_root(self) -> str | None: | ||
| filename = getattr(self.editor, "filename", None) | ||
| if filename: | ||
| path = Path(str(filename)).expanduser() | ||
| return str(path.parent if path.suffix else path) | ||
| registry = getattr(self.editor, "service_registry", None) | ||
| project_service = getattr(registry, "project_service", None) | ||
| root = getattr(project_service, "root", None) | ||
| return str(root) if root else None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prefer the project service root for workspace diagnostics.
When editor.filename is set, Line 2466 uses the active file’s parent and never reaches project_service.root; workspace diagnostics then display relative to the current file’s directory instead of the project root. Use the service root first, with the filename parent as fallback.
Suggested fix
def _diagnostics_project_root(self) -> str | None:
- filename = getattr(self.editor, "filename", None)
- if filename:
- path = Path(str(filename)).expanduser()
- return str(path.parent if path.suffix else path)
registry = getattr(self.editor, "service_registry", None)
project_service = getattr(registry, "project_service", None)
root = getattr(project_service, "root", None)
- return str(root) if root else None
+ if root:
+ return str(root)
+ filename = getattr(self.editor, "filename", None)
+ if filename:
+ path = Path(str(filename)).expanduser()
+ return str(path.parent if path.suffix else path)
+ return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _diagnostics_project_root(self) -> str | None: | |
| filename = getattr(self.editor, "filename", None) | |
| if filename: | |
| path = Path(str(filename)).expanduser() | |
| return str(path.parent if path.suffix else path) | |
| registry = getattr(self.editor, "service_registry", None) | |
| project_service = getattr(registry, "project_service", None) | |
| root = getattr(project_service, "root", None) | |
| return str(root) if root else None | |
| def _diagnostics_project_root(self) -> str | None: | |
| registry = getattr(self.editor, "service_registry", None) | |
| project_service = getattr(registry, "project_service", None) | |
| root = getattr(project_service, "root", None) | |
| if root: | |
| return str(root) | |
| filename = getattr(self.editor, "filename", None) | |
| if filename: | |
| path = Path(str(filename)).expanduser() | |
| return str(path.parent if path.suffix else path) | |
| return None |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/ui/panels.py` around lines 2462 - 2470, The
_diagnostics_project_root helper currently prefers editor.filename and returns
the active file’s directory before considering project_service.root, which
causes workspace diagnostics to anchor to the wrong location. Update
_diagnostics_project_root in the panels.py logic to read
service_registry.project_service.root first, and only fall back to deriving a
path from editor.filename when the project root is unavailable.
| def _sync_selected_diagnostic_highlight( | ||
| self, | ||
| snapshot: DiagnosticsSnapshot | None, | ||
| diagnostics: tuple[Diagnostic, ...], | ||
| ) -> None: | ||
| if snapshot is None or snapshot.status != "ready" or not diagnostics: | ||
| self._clear_diagnostic_line_highlight() | ||
| return | ||
| diagnostic = self._selected_diagnostic() | ||
| if diagnostic is None: | ||
| self._clear_diagnostic_line_highlight() | ||
| return | ||
| setter = getattr(self.editor, "set_diagnostic_line_highlight", None) | ||
| if callable(setter): | ||
| setter(diagnostic, generation=snapshot.generation) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep highlight selection tied to the supplied snapshot.
_sync_selected_diagnostic_highlight() accepts a snapshot and diagnostics tuple, but Line 2616 re-reads the current snapshot through _selected_diagnostic(). Use the passed tuple so the diagnostic and generation=snapshot.generation cannot drift across async refresh boundaries.
Suggested fix
- diagnostic = self._selected_diagnostic()
- if diagnostic is None:
- self._clear_diagnostic_line_highlight()
- return
+ idx = max(0, min(len(diagnostics) - 1, self.selected_idx))
+ self.selected_idx = idx
+ diagnostic = diagnostics[idx]
setter = getattr(self.editor, "set_diagnostic_line_highlight", None)
if callable(setter):
setter(diagnostic, generation=snapshot.generation)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _sync_selected_diagnostic_highlight( | |
| self, | |
| snapshot: DiagnosticsSnapshot | None, | |
| diagnostics: tuple[Diagnostic, ...], | |
| ) -> None: | |
| if snapshot is None or snapshot.status != "ready" or not diagnostics: | |
| self._clear_diagnostic_line_highlight() | |
| return | |
| diagnostic = self._selected_diagnostic() | |
| if diagnostic is None: | |
| self._clear_diagnostic_line_highlight() | |
| return | |
| setter = getattr(self.editor, "set_diagnostic_line_highlight", None) | |
| if callable(setter): | |
| setter(diagnostic, generation=snapshot.generation) | |
| def _sync_selected_diagnostic_highlight( | |
| self, | |
| snapshot: DiagnosticsSnapshot | None, | |
| diagnostics: tuple[Diagnostic, ...], | |
| ) -> None: | |
| if snapshot is None or snapshot.status != "ready" or not diagnostics: | |
| self._clear_diagnostic_line_highlight() | |
| return | |
| idx = max(0, min(len(diagnostics) - 1, self.selected_idx)) | |
| self.selected_idx = idx | |
| diagnostic = diagnostics[idx] | |
| setter = getattr(self.editor, "set_diagnostic_line_highlight", None) | |
| if callable(setter): | |
| setter(diagnostic, generation=snapshot.generation) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/ui/panels.py` around lines 2608 - 2622, Keep the selected diagnostic
highlight tied to the snapshot passed into _sync_selected_diagnostic_highlight()
instead of re-reading state via _selected_diagnostic(). Use the supplied
diagnostics tuple to resolve the selected Diagnostic, then pass that same object
to set_diagnostic_line_highlight with generation=snapshot.generation so the
highlight cannot drift across async refreshes.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/ecli/diagnostics/service.py`:
- Around line 183-204: The aggregate result in DiagnosticService is mixing
messages from different provider statuses, so a later ready/pass message can
override an earlier error/skipped status. Update the flow around
_merge_provider_result, _final_result_status, and _final_result_message to keep
messages tied to their originating status, either by tracking separate buckets
for error/skipped/ready messages or by storing (status, message) pairs and
selecting the last message that matches the final status.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f74f8575-d4d0-446c-abc9-4f555f9dbbf2
📒 Files selected for processing (5)
src/ecli/core/Ecli.pysrc/ecli/diagnostics/service.pysrc/ecli/ui/DrawScreen.pysrc/ecli/ui/panels.pytests/ui/test_diagnostics_panel.py
| status: DiagnosticStatus = "ready" | ||
| saw_skipped = False | ||
| for provider in enabled: | ||
| status, saw_skipped = self._merge_provider_result( | ||
| provider.run(request), | ||
| diagnostics, | ||
| messages, | ||
| status, | ||
| saw_skipped, | ||
| ) | ||
|
|
||
| sorted_diagnostics = sort_diagnostics(diagnostics) | ||
| status = self._final_result_status( | ||
| status, | ||
| has_diagnostics=bool(sorted_diagnostics), | ||
| saw_skipped=saw_skipped, | ||
| ) | ||
| message = self._final_result_message( | ||
| status, | ||
| diagnostic_count=len(sorted_diagnostics), | ||
| messages=messages, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep provider messages tied to their status.
If one provider returns status="error" and a later provider returns a PASS/ready message, the aggregate status remains "error" but Line 256 displays the later non-error message. Track error/skipped messages separately, or store (status, message) pairs and select the last message matching the final status.
Suggested direction
- messages: list[str] = []
+ messages: list[tuple[DiagnosticStatus, str]] = []
...
- if result.message:
- messages.append(result.message)
+ if result.message:
+ messages.append((result.status, result.message))
...
- return messages[-1] if messages else "Diagnostics failed."
+ return _last_message(messages, "error") or "Diagnostics failed."
...
- return messages[-1] if messages else "Diagnostics skipped."
+ return _last_message(messages, "skipped") or "Diagnostics skipped."Also applies to: 227-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ecli/diagnostics/service.py` around lines 183 - 204, The aggregate result
in DiagnosticService is mixing messages from different provider statuses, so a
later ready/pass message can override an earlier error/skipped status. Update
the flow around _merge_provider_result, _final_result_status, and
_final_result_message to keep messages tied to their originating status, either
by tracking separate buckets for error/skipped/ready messages or by storing
(status, message) pairs and selecting the last message that matches the final
status.
1 participant
This PR implements the F4 Diagnostics / Linter Panel milestone.
Changes:
Runtime behavior:
Evidence:
Manual smoke confirmed:
Validation:
Final broad validation:
Non-goals:
Summary by CodeRabbit